Skip to content

MDEV-39880 Reïmplement MDEV-37146 to include MDEV-39519 - #5458

Open
ParadoxV5 wants to merge 1 commit into
13.0from
MDEV-39880
Open

MDEV-39880 Reïmplement MDEV-37146 to include MDEV-39519#5458
ParadoxV5 wants to merge 1 commit into
13.0from
MDEV-39880

Conversation

@ParadoxV5

@ParadoxV5 ParadoxV5 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

MDEV-39519 added MySQL compatibility to mariadb-dump --dump-slave by attempting SHOW REPLICA STATUS first and then SHOW SLAVE STATUS.
This conflicted with MDEV-37146, where mariadb-dump --dump-slave queries either SELECT … FROM information_schema.SLAVE_STATUS or SHOW ALL SLAVES STATUS depending on the server version.

This commit merges MDEV-37146 and MDEV-39519:

  • Use MDEV-39519’s strategy based on syntax error handling.
  • Use MDEV-37146’s preference order:
    1. SELECT … FROM information_schema.SLAVE_STATUS
    2. SHOW ALL SLAVES STATUS
    3. SHOW REPLICA STATUS (for MySQL compatibility only)
    • Send STOP/START REPLICA SQL_THREAD FOR CHANNEL '…' commands for both MariaDB (est. 10.7) and MySQL.
  • Refactor column indices to variables set when a query succeeds.
  • Partially revert MDEV-37146’s removal of --dump-slave’s support for pre-GTID & pre-multi-source, but tailored for MySQL compatibility; coverage for MariaDB pre-10.0 is not fully restored.
    (That would require a 4th command: SHOW SLAVE STATUS.)

Disclaimer re. «Consistent use of terminology "master" and "slave" in replication development»:

All appearances of REPLICA in this PR are about MySQL compatibility and are not directly related to… that movement.

@ParadoxV5
ParadoxV5 requested a review from bnestere July 27, 2026 00:49
@ParadoxV5 ParadoxV5 added MariaDB Corporation Replication Patches involved in replication labels Jul 27, 2026
@gemini-code-assist

This comment was marked as spam.

Comment thread client/mysqldump.cc
@@ -6603,16 +6635,18 @@ static int do_show_slave_status(MYSQL *mysql_con,
if (use_gtid)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 10.11: Warn when using --gtid to dump MySQL or a pre-GTID MariaDB?

The behaviour before MDEV-37146 or MDEV-39519 appears to still dump no-MariaDB-GTID servers with the following snippet, just not followed by SET GLOBAL gtid_slave_pos='…';.

-- The deferred gtid setting for slave corresponding to the dump-slave CHANGE-MASTER follows
-- GTID position to start replication:

Comment thread client/mysqldump.cc
have_info_schema_slave_status= false;
// Between 10.0 and 11.5 inclusive
switch (mysql_query_with_error_report(mysql_con, &slave_status_res,
"SHOW ALL SLAVES STATUS", true

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered using REPLICAS here as well, but I noticed that if MySQL suddenly adds SHOW ALL REPLICAS STATUS, but in MySQL’s column order rather than MariaDB’s, then these assumptions would break in the absence of version checks.

(It’s not MDEV-39880’s concern, but version ≥ 10.0 checks probably will also fall apart soon with the upcoming MySQL 26.7.
They have not released their last quarter’s commits, so the only way to verify is to download their entire pre-release, which I’m not bothering.)

On the other hand, I assume that both MariaDB deleting the SLAVES keyword and MySQL reviving it are very unlikely.

Comment thread client/mysqldump.cc
if (mysql_query_with_error_report(mysql_con, &slave_status_res,
have_info_schema_slave_status ?
// 11.6 and above
switch (mysql_query_with_error_report(mysql_con, &slave_status_res,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m tempted to revive my old draft implementation based on virtual methods.
But the old argument holds that we will eventually deprecate 11.4 & before.
It’s also really MySQL’s problem that their SQL version of SHOW REPLICA STATUS is still stuck behind Perf. Schema.


I did not extend mysql_query_with_retry() because this code design requires an overly detailed flexibility.
(Although it’d be cleaner with that virtual methods code design.)


Currently, the only other uses of mysql_query_with_retry() are in --master-data ’s and --delete-master-logs’s code, but they both query SHOW MASTER STATUS first and then SHOW BINARY LOG STATUS.

It’s not my current focus to get them to share their results, though (but it’s viable because --delete-master-logs includes --master-data).

Side note

While I removed --dump-slave & --apply-slave-statements support for pre-10.0 in MDEV-37146, I did not touch --master-data.

Comment thread client/mysqldump.cc

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR does not add --apply-slave-statements support for MySQL.
But I doubt that mariadb-dump should support outputting for MySQL.

STOP/START REPLICA also means STOP/START ALL SLAVES on MySQL, but not on MariaDB, so this is a (if not the only) detail where an output intended for MySQL would have an incomplete effect on MariaDB.

@ParadoxV5 ParadoxV5 Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… which reminds me that I must use STOP/START REPLICA SQL_THREAD FOR CHANNEL '…', which is compatible with both MariaDB 10.7 and MySQL.
fixing now done

[P.S.] 10.6 isn’t dead dead tho…
[P.P.S] … eh screw it for today; ES and other extended supporters can add their own code branch or backport FOR CHANNEL to their 10.6.

So should the MDEV-39519 base…
But the old code design can only send the base STOP/START REPLICAL SQL_THREAD to MySQL, which works, just inconvenient.
Oh well.

MDEV-39519 added MySQL 8 compatibility to `mariadb-dump --dump-slave`
by attempting `SHOW REPLICA STATUS` first and then `SHOW SLAVE STATUS`.
This conflicted with MDEV-37146, where `mariadb-dump --dump-slave`
queries either `SELECT … FROM information_schema.SLAVE_STATUS`
or `SHOW ALL SLAVES STATUS` depending on the server version.

This commit merges MDEV-37146 and MDEV-39519:
* Use MDEV-39519’s strategy based on syntax error handling.
* Use MDEV-37146’s preference order:
  1. `SELECT … FROM information_schema.SLAVE_STATUS`
  2. `SHOW ALL SLAVES STATUS`
  3. `SHOW REPLICA STATUS` (for MySQL compatibility _only_)
* Send `STOP`/`START REPLICA SQL_THREAD FOR CHANNEL '…'`
  commands for both MariaDB (est. 10.7) and MySQL.
* Refactor column indices to variables set when a query succeeds.
* Partially revert MDEV-37146’s removal of `--dump-slave`’s support for
  pre-GTID & pre-multi-source, but tailored for MySQL compatibility;
  coverage for MariaDB pre-10.0 is not fully restored.

@bnestere bnestere left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ParadoxV5 !

Thanks for working on this. Please see my notes.

Comment thread client/mysqldump.cc
return(1);
" WHERE Slave_SQL_Running <> 'No'", true
)) {
default:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default should conventionally be last in a switch-case. Though I suppose you did it here to keep case -1 last to make it easier to read this series of switch/if nests. I think that's fine, though any time you break convention, please add a comment to justify why. Don't assume it will be obvious for others :)

Comment thread client/mysqldump.cc
NAME_CHAR_LEN, row[/* Connection_name */ 0]);
char query[39 + NAME_CHAR_LEN]; // sizeof(snprintf)
snprintf(query, sizeof(query),
"STOP REPLICA SQL_THREAD FOR CHANNEL '%.*s'", // 10.7+ or MySQL

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use FOR CHANNEL? Won't this break when running on MariaDB 10.6 (still supported)?

Comment thread client/mysqldump.cc
}

if (get_gtid_pos(gtid_pos, false))
if (have_mariadb_gtid)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a bug fix for MySQL? It looks like, before, against MySQL, we would error and return here. Does the rest of the dump actually work against MySQL? If not, it is probably better to error than to silently output commands which won't actually work.

Alternatively, you mention

For 10.11: Warn when using --gtid to dump MySQL or a pre-GTID MariaDB?

I'd say this is error-worthy rather than warn.

Comment thread client/mysqldump.cc
ptrdiff_t connection_name, slave_sql_running,
master_host, master_port,
relay_master_log_file, exec_master_log_pos;
} slave_status_indices;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat pattern here, I like this

Comment thread client/mysqldump.cc
mysql_free_result(slave);
return 1;
}
slave_status_indices= {0, /* unused */ 3, 1, 2, 3, 4};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is slave_sql_running assigned to 3 if it is unused? I'd suggest making it an obviously wrong number (like max ptrdiff_t).

Comment thread client/mysqldump.cc
if (mysql_query_with_error_report(mysql_con, &slave_status_res,
have_info_schema_slave_status ?
// 11.6 and above
switch (mysql_query_with_error_report(mysql_con, &slave_status_res,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mysql_query_with_error_report returns -1 only on ER_PARSE_ERROR. I don't think it will fire when missing the information_schema.slave_status. You likely have to extend mysql_query_with_error_report() to return -1 also on missing slave_status.

Comment thread client/mysqldump.cc
char query[39 + NAME_CHAR_LEN]; // sizeof(snprintf)
snprintf(query, sizeof(query),
"STOP REPLICA SQL_THREAD FOR CHANNEL '%.*s'", // 10.7+ or MySQL
NAME_CHAR_LEN, row[slave_status_indices.connection_name]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slave_status_indices.connection_name isn't initialized here when have_info_schema_slave_status. Though note this isn't true for the parent access to slave_status_indices:

    if ((have_info_schema_slave_status ||
        strcmp(row[slave_status_indices.slave_sql_running], "No")))

which first checks have_info_schema_slave_status.

I think the information_schema.slave_status column name strings & its slave_status_indices should be initialized together.

@bnestere

Copy link
Copy Markdown
Contributor

@ParadoxV5 One more thing I forgot to mention in my review - we should have this manually tested across old MariaDB and various MySQL versions. Please coordinate that with @mariadb-SusilBehera.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

MariaDB Corporation Replication Patches involved in replication

Development

Successfully merging this pull request may close these issues.

2 participants